Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Load OGR layers in parallel when opening a project #58337

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Load OGR layers in parallel when opening a project #58337

wants to merge 3 commits into from

Conversation

wonder-sk
Copy link
Member

Parallel loading of GDAL and PostgreSQL layers (when opening a QGIS project) has been added in QGIS 3.32 (#53069) - this is a follow up that enables parallel loading of OGR layers.

In my testing scenario with a QGIS project with 7 remote GeoPackages (using vsicurl), the time to load map layers went down from ~5.5 seconds to ~1.5 seconds.

There are three commits that introduce it:

  1. 5c94938 - enable the flag to allow loading of OGR data providers in parallel
  2. 6630c22 - fix an existing deadlock in QGIS when creating multiple OGR providers at once
  3. 5a94278 - update locking mechanism so that datasets can be actually loaded in parallel (rather than serialized due to locking)

@wonder-sk
Allow creation of OGR providers in parallel (when opening projects)
5c94938
@wonder-sk
Fix a deadlock when loading multiple OGR providers at once
6630c22 
There was a deadlock between QgsOgrProvider's loadMetadata() and open() functions:
- loadMetadata() would first lock data source related mutex (QgsOgrProviderUtils::DatasetWithLayers::mutex)
  and then lock the global mutex (sGlobalMutex)
- the open() function would call QgsOgrProviderUtils::getLayer() where the order of locking is reversed,
  i.e. first lock the global mutex, then the data source related mutex

Fixed by updating loadMetadata() to lock data source related mutex for the minimum time
necessary (certainly before the global mutex gets locked).
@wonder-sk
Allow opening of GDAL datasets in OGR provider in parallel
5a94278 
Until now, creation of GDAL datasets from within OGR provider would get
serialized, because QgsOgrProviderUtils::getLayer() starts by locking
a global mutex. This is now changed to lock only for the time while
shared data structures need to be accessed, and therefore it is possible
that multiple threads can open GDAL datasets in parallel.
@wonder-sk wonder-sk changed the title Loading OGR layers in parallel when opening a project Load OGR layers in parallel when opening a project Aug 7, 2024
@github-actions github-actions bot added this to the 3.40.0 milestone Aug 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2024 •
edited
Loading

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 5a94278)

rouault
rouault reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@rouault rouault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Wondering if there would be ways of adding tests for multithreaded dataset opening? (perhaps requires C++ test if the GIL is not released in Python)

@nyalldawson nyalldawson reopened this Aug 17, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 1, 2024

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Sep 9, 2024
@wonder-sk wonder-sk reopened this Sep 16, 2024
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 16, 2024
@nyalldawson nyalldawson added the Freeze Exempt Feature Freeze exemption granted label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault left review comments

Assignees
No one assigned
Labels
Freeze Exempt Feature Freeze exemption granted
Projects
None yet
Milestone
3.40.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wonder-sk @rouault @nyalldawson